Support using flag --skipslow instead of -m "not slow" for pytest#1421
Support using flag --skipslow instead of -m "not slow" for pytest#1421mhucka wants to merge 7 commits into
--skipslow instead of -m "not slow" for pytest#1421Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new --skipslow flag for pytest to improve test execution control, updating the CONTRIBUTING.md documentation, pyproject.toml configuration, and conftest.py setup. The reviewer recommended using more specific type hints (pytest.Parser and pytest.Item) and the getoption API instead of getvalue for better type safety and correctness.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a custom --skipslow command-line option for pytest to skip slow tests, replacing the previous -m "not slow" marker-based filtering. It updates the documentation, pytest configuration, and adds unit tests for the new conftest hooks. The review feedback points out that checking item.keywords for "slow" is a pytest anti-pattern because it can match test names or module names containing the word "slow", and suggests using item.get_closest_marker("slow") instead, along with updating the corresponding unit tests to mock this method.
| def pytest_runtest_setup(item: pytest.Item) -> None: | ||
| if "slow" in item.keywords and item.config.getoption("skipslow"): | ||
| pytest.skip("skipped because of --skipslow option") |
There was a problem hiding this comment.
Using "slow" in item.keywords is a pytest anti-pattern. item.keywords contains not only the markers applied to a test, but also the names of the test function, class, and module (and their components split by underscores). This means any test with the word slow in its name, class, or file name (e.g., test_slow_algorithm) will be incorrectly skipped when --skipslow is passed, even if it is not marked with @pytest.mark.slow.
Instead, use item.get_closest_marker("slow") to safely and precisely check for the presence of the marker.
| def pytest_runtest_setup(item: pytest.Item) -> None: | |
| if "slow" in item.keywords and item.config.getoption("skipslow"): | |
| pytest.skip("skipped because of --skipslow option") | |
| def pytest_runtest_setup(item: pytest.Item) -> None: | |
| if item.get_closest_marker("slow") is not None and item.config.getoption("skipslow"): | |
| pytest.skip("skipped because of --skipslow option") |
| def test_pytest_runtest_setup_skips(self): | ||
| import conftest | ||
| import pytest | ||
| from unittest.mock import MagicMock | ||
|
|
||
| # Create mock item representing a slow test when skipslow is True. | ||
| item = MagicMock() | ||
| item.keywords = {"slow"} | ||
| item.config.getoption.return_value = True | ||
|
|
||
| with self.assertRaises(pytest.skip.Exception): | ||
| conftest.pytest_runtest_setup(item) | ||
|
|
||
| item.config.getoption.assert_called_once_with("skipslow") | ||
|
|
||
| def test_pytest_runtest_setup_does_not_skip_if_not_slow(self): | ||
| import conftest | ||
| from unittest.mock import MagicMock | ||
|
|
||
| # Test case 1: Not marked as 'slow', skipslow is True. | ||
| item = MagicMock() | ||
| item.keywords = set() | ||
| item.config.getoption.return_value = True | ||
|
|
||
| # Should not raise an exception. | ||
| conftest.pytest_runtest_setup(item) | ||
|
|
||
| # Test case 2: Marked as 'slow', skipslow is False. | ||
| item = MagicMock() | ||
| item.keywords = {"slow"} | ||
| item.config.getoption.return_value = False | ||
|
|
||
| conftest.pytest_runtest_setup(item) |
There was a problem hiding this comment.
Since we are updating pytest_runtest_setup to use item.get_closest_marker("slow") instead of checking item.keywords, we should update the unit tests to mock get_closest_marker accordingly.
def test_pytest_runtest_setup_skips(self):
import conftest
import pytest
from unittest.mock import MagicMock
# Create mock item representing a slow test when skipslow is True.
item = MagicMock()
item.get_closest_marker.return_value = MagicMock()
item.config.getoption.return_value = True
with self.assertRaises(pytest.skip.Exception):
conftest.pytest_runtest_setup(item)
item.get_closest_marker.assert_called_once_with("slow")
item.config.getoption.assert_called_once_with("skipslow")
def test_pytest_runtest_setup_does_not_skip_if_not_slow(self):
import conftest
from unittest.mock import MagicMock
# Test case 1: Not marked as 'slow', skipslow is True.
item = MagicMock()
item.get_closest_marker.return_value = None
item.config.getoption.return_value = True
# Should not raise an exception.
conftest.pytest_runtest_setup(item)
# Test case 2: Marked as 'slow', skipslow is False.
item = MagicMock()
item.get_closest_marker.return_value = MagicMock()
item.config.getoption.return_value = False
conftest.pytest_runtest_setup(item)
pavoljuhas
left a comment
pavoljuhas
left a comment
There was a problem hiding this comment.
Please remove the LLM code-salad ConftestTest.
The second comment to use the pytest_collection_modifyitems is optional, feel free to leave it as is with a bit less accurate skip report.
Otherwise LGTM.
| def pytest_runtest_setup(item: pytest.Item) -> None: | ||
| if "slow" in item.keywords and item.config.getoption("skipslow"): | ||
| pytest.skip("skipped because of --skipslow option") |
There was a problem hiding this comment.
This reports conftest.py as the source file for the skipped tests:
$ pytest -rs --skipslow src/openfermion/ops/representations/doci_hamiltonian_test.py::IntegralTransformsTest::test_fermionic_hamiltonian_from_integrals
...
================================================== short test summary info ===================================================
SKIPPED [1] conftest.py:77: skipped because of --skipslow option
===================================================== 1 skipped in 0.06s =====================================================I think it is better to use the pytest_collection_modifyitems hook as in cirq which reports the actual test skipped:
$ pytest -rs dev_tools/notebooks/notebook_test.py
...
================================================== short test summary info ===================================================
SKIPPED [59] dev_tools/notebooks/notebook_test.py:101: need --enable-slow-tests option to run
=============================================== 2 passed, 59 skipped in 0.17s ================================================
| class ConftestTest(unittest.TestCase): | ||
|
|
||
| def test_pytest_addoption(self): | ||
| import conftest | ||
| from unittest.mock import MagicMock | ||
|
|
||
| parser = MagicMock() | ||
| conftest.pytest_addoption(parser) | ||
| parser.addoption.assert_called_once_with( | ||
| "--skipslow", action="store_true", help="skips slow tests" | ||
| ) |
There was a problem hiding this comment.
There seems to be too much mockery and AI generated crud for this test to be meaningful.
It verifies that conftest hook functions do what they do on their arguments, but that tells nothing of if the hooks are used in a pytest session and if they have desired effects.
I suggest to delete this; it is a second order test-of-a-test-code anyway.
2 participants
The flag
-m "not slow"was not only annoying to type: it invited misspelling and waste of time.This PR changes the flag to be
--skipslow, following the same thing done in the ReCirq project.